- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 1.4k
 
chore: Change SVGBrush representation from number or string to object #8414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
34183be    to
    ef2ae7c      
    Compare
  
    36afb32    to
    6495352      
    Compare
  
    | } | ||
| 
               | 
          ||
| return { | ||
| return () => ({ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it needs to be an function instead of an object? I don't see a clear reason why 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some processors like this one:
export const processMarginHorizontal: ValueProcessor<DimensionValue> = (
  value
) => {
  const result = parseDimensionValue(value);
  if (!result) {
    return;
  }
  return () => ({
    marginLeft: result,
    marginRight: result,
  });
};They split a single property into multiple ones as I expect only marginLeft, marginRight, marginTop and marginBottom in CPP and don't want to pass others like marginHorizontal, etc. This reduces unnecessary complexity but makes it impossible to return and object from the value processor as it will be interpreted as separate properties. To handle this case, I decided to return a function for processors which result should be considered as separate properties instead of a single object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit tricky, but I see why you used this approach. The most readable solution would be to add an additional property that describes the type of data, but this would introduce extra overhead. What do you think would be more beneficial for us in the long term - the current solution with lambdas or adding an additional property type to the object?
| SVGBrush::SVGBrush(const folly::dynamic &dynamicValue) | ||
| : CSSColorBase<SVGBrushType, SVGBrush>(SVGBrushType::Transparent) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small nitpick: since you already have that constructor in CSSBaseColor:
template <ColorTypeEnum TColorType, typename TDerived>
CSSColorBase<TColorType, TDerived>::CSSColorBase()
    : channels{0, 0, 0, 0}, colorType(TColorType::Transparent) {}I think here you could just call:
SVGBrush::SVGBrush(const folly::dynamic &dynamicValue)
    : CSSColorBase<SVGBrushType, SVGBrush>() {| bool SVGBrush::canConstruct(const folly::dynamic &value) { | ||
| return value.isNumber() || value.empty() || | ||
| isValidColorString(value.asString()); | ||
| colorType = type; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me why we only assign the color type when it's different from RGBA. This could potentially be problematic in the future then someone will add logic based on color type because the default color type value here is Transparent, but the actual value is RGBA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to introduce unnecessary complexity with variants so I decided that all colors have 4 RGBA channels and the color type. If the type is different from the RGBA type, the color channels can be ignored.
Currently supported remaining color types for SVGs are Transparent and CurrentColor. Both of them cannot be represented as RGBA values, since the Transparent color depends on another one in the interpolation pair (the color we interpolate from/to) and the CurrentColor is treated as a keyword (discrete value) as I can't obtain the current color easily in reanimated (at least I don't know how) and it is managed by SVG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What else do you suggest? Should I make the color channels value optional or introduce a variant for the value and either store 4 RGBA channels or a keyword/sth else indicating that the color is Transparent/CurrentColor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if i miss understand, but I thought that we can just do it like that:
SVGBrush::SVGBrush(const folly::dynamic &dynamicValue)
    : CSSColorBase<SVGBrushType, SVGBrush>(SVGBrushType::Transparent) { 1Code has comments. Press enter to view.
  const auto [type, value] = parseDynamicValue(dynamicValue);
   colorType = type; // <-- just always assign the type, no mater if it is a SVGBrushType::Rgba / Transparent etc 
  if (type == SVGBrushType::Rgba) {
    *this = SVGBrush(value.asInt());
    return;
  }I don't want to introduce variants too - this is not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. Sorry for misunderstanding. Yes, we can do this but it is not necessary as the SVGBrush constructor for numbers already assigns the SVGBrushType::Rgba type, so we will end up with assigning the same value twice. Both solutions are correct, though.
ef2ae7c    to
    4ed7ecc      
    Compare
  
    
Summary
This PR changes the format of the SVG brush value passed from JS to CPP in CSS animations from the single value to the object with
colorTypeandvalueprops. This will align better with SVG's implementation of the brush type.